Skip to content

Conversation

@jchlanda
Copy link
Contributor

@jchlanda jchlanda commented Dec 12, 2024

The need for updated check was caused by a move from using AMD builtins in favour of IR instructions with correct metadata attached, see: b5e63cc

I travelled back in time to:

and verified that generated assembly (cpp -> IR -> s) is the same for before and after the builtin change.

@jchlanda jchlanda requested a review from a team as a code owner December 12, 2024 10:46
@jchlanda jchlanda requested a review from ldrumm December 12, 2024 10:46
@jchlanda
Copy link
Contributor Author

FIXES: #15377

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking the ! might help make it clear we're checking for metadata.

Alternatively, we could maybe just check the whole line, without skipping anything ({{.*}})? I take it we can't use update_cc_test_checks.py to autogenerate everything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the !, seems like update tests script is having a hard time trying to parse the output of the tests:

!93 = !{!13, !32}
Traceback (most recent call last):
  File "./llvm/utils/update_cc_test_checks.py", line 561, in <module>
    sys.exit(main())
  File "./llvm/utils/update_cc_test_checks.py", line 392, in main
    for k, v in get_line2func_list(ti.args, clang_args).items():
  File "./llvm/utils/update_cc_test_checks.py", line 127, in get_line2func_list
    ast = json.loads(stdout)
  File "/usr/lib64/python3.6/json/__init__.py", line 354, in loads
    return _default_decoder.decode(s)
  File "/usr/lib64/python3.6/json/decoder.py", line 342, in decode
    raise JSONDecodeError("Extra data", s, end)
json.decoder.JSONDecodeError: Extra data: line 23586733 column 2 (char 1320540514)

In any case not sure if it would bring that much.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aw, fair enough. Just that automating the tests is like clang-format (don't have to think about it) and means we'd catch any unwanted change to the output, like if the alignment was wrong, or an update brought in extra metadata that we don't expect.

We should probably look at fixing that script for SYCL (if that is indeed the problem) - I think it would help the upstreaming effort.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added it to the internal issue tracker.

@jchlanda
Copy link
Contributor Author

CUDA failure is not related, but will re-run the tests.

@jchlanda
Copy link
Contributor Author

@ldrumm if you're happy with the patch, it should be ready to roll.

@ldrumm ldrumm merged commit f15e86b into intel:sycl Dec 16, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants